Repository navigation
Warn about directives applied where their definition doesn't allow them - #885
Open
oryan-block wants to merge 1 commit into
Open
oryan-block wants to merge 1 commit into
oryan-block wants to merge 1 commit into
Conversation
Logs a warning for a directive applied at a location its definition doesn't list, without a required argument, or more than once when it isn't repeatable. The schema still builds as before. Arguments of directive definitions now get the ARGUMENT_DEFINITION location instead of OBJECT. Fixes #225
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #225
Checklist
Description
Three kinds of wrong directive usage build without a word today:
graphql-java's
SchemaGeneratorrejects all three, but tools doesn't use it. The schema validation that does run on our schema doesn't catch them either: it checks repeats only on the deprecatedGraphQLDirectives, whichbuildDirectivesalready de-dupes, and it never checks locations or missing arguments.These schemas always built, so for now they log a WARN and keep building as before. Throwing instead is #884, for 15.0.0:
The checks live in
buildAppliedDirectives, which now gets the location and a name for the element (type 'Book',argument 'id' of field 'Query.book',the schema, ...). They run against theGraphQLDirectivedefinition, so built-in directives are checked too (@deprecatedon a type warns). Undeclared directives allowed byallowUndeclaredDirectiveshave nothing to check against and are skipped. A repeat that a type extension adds is still handled by the existing warning indirectivesOf, so it isn't reported twice.Every element used to call
withAppliedDirectives(*buildAppliedDirectives(..))followed bywithDirectives(*buildDirectives(.., LOCATION)), 10 times. That's now onewithDirectivesAt(directives, LOCATION, element)per site, so the location is written once. The directive definition arguments were passingOBJECTthere, which is nowARGUMENT_DEFINITION. That only changes the location on the deprecatedGraphQLDirectives attached to those arguments, and nothing in tools reads it.createFieldandcreateArgumentget the type name for the messages.Two test fixtures warned:
DirectiveTest's@uppercase on OBJECTapplied to a field, andRelayConnection.graphqls's@connection(for: String!) on FIELD.FIELDis a location for queries, not for the schema, so both now useFIELD_DEFINITION. The relay docs don't show a declaration, so I'd guess some users copied theon FIELDone from our tests. They'll start seeing the warning.Added a
DirectiveTestcase with all three checks, a built-in directive and the schema, asserting the exact warnings and that the repeats are still kept. The default values test now asserts no warnings, which covers the schema, enum value, input field and argument sites.TypeExtensionTest's exact warning list didn't change, which covers the type sites and the extension repeat. I also traced every check across the whole suite: only the two fixtures above warned.mvn clean verifypasses on JDK 25, on JDK 25 against graphql-java 25.1, and on JDK 11.Also checked through the public API with a schema like the one above: master logs nothing, this branch logs the three warnings (plus
@deprecatedon a type and a repeated schema directive). The printed schema and the query result are identical on both, and@uppercasestill uppercases through its wiring. graphql-java's own checker finds no directive problems in a large real schema (107 SDL files), so I don't expect much noise.Release notes: schemas that misuse directives now log a WARN from
graphql.kickstart.tools.SchemaParser. For@connection, declare iton FIELD_DEFINITION.